FIX(client): Positional audio fixes #6234

Hartmnt · 2023-10-10T23:13:58Z

This MR contains 3 commits, each related to problems in the current implementation of positional audio.

The problems that are fixed are:

The manual position plugin listener indicator was facing the wrong direction
The sliders for minimum and maximum positional distance did not properly load values
The use case for minimum volume = 0 is broken

src/mumble/AudioOutput.cpp

…indicator Previously, the ManualPlacement plugin for the users position/orientation was pointing in the wrong (opposite) direction. This fix swaps the orientation of the sprite. It also set the default azimuth to 0 instead of 180 to make the orientation of the listener intuitive for the user by default. The new default azimuth will not change the logic of the plugin whatsoever.

Hartmnt · 2023-10-11T12:49:06Z

@Krzmbrzl I have added all your change requests. ~~Interestingly the 0.75 does not seem to influence the audio result at all, which is good because I was afraid I had to fiddle with the other constants again.~~ But now it should be numerically sound. Meh, I increased the clamping in calcGain to 0.005 from 0.003 after testing some more...

I have also noticed that simply enabling positional audio would make audio sources ever so slightly quieter. I assume this is because the speakers have a natural distance of a few centimeters from the player, which then influences the attenuation even though the player is exactly within the audio source.
Since it is obviously not possible in real life to be exactly inside an audio source, and I assume games might place players in the same location (for example when they are dead and spectate the game), I have added another case to calcGain where it simply returns 1.0f when the distance is numerically 0.

src/mumble/AudioOutput.cpp

Krzmbrzl · 2023-10-12T07:02:16Z

As mentioned in #6149 (comment), I think we can implement the minimum volume on opposite ear effect a bit more elegantly by shifting our dot-product slightly:
While renormalizing the dot product into the range [0,1] such that speakers on the opposite direction of the sound source end up producing a dot product of zero (resulting in no audio output there, according to calcGain), we can ensure that the dot-product never actually reaches zero, equating to the directional damping of volume will never completely mute the audio.
This seems to be what we are currently doing as a post-processing step, where we add the constant shift of 0.25 * maxVolume.

So if we simply did dot = std::min(dot, 0.25), this should have the same effect, but without the additional code-complexity for the post-processing, shouldn't it?

I suppose we could even do things a bit more fancy by "fading in" the constant offset, depending on how close the audio source is to being muted:

dot = <renormalization>;
offset = (1 - dot) * 0.25;
dot += offset;

This should give an additional effect where changes in direction close to being antiparallel to a given speaker should still have an effect on the volume of the audio from that speaker (whether or not that is actually audible is another question though) in contrast to the simple std::min approach, where the volume would plateau at 0.25 until that value is exceeded.

Does this all make some sense? 🤔

EDIT: Note that this is merely a suggestion / an idea I had and not something I definitely want to be done in that way. View it as a request for your opinion :)

Hartmnt · 2023-10-12T15:27:26Z

dot = std::min(dot, 0.25)

Should be std:max. Do not ask me how I know.

I suppose we could even do things a bit more fancy by "fading in" the constant offset, depending on how close the audio source is to being muted:
dot = <renormalization>;
offset = (1 - dot) * 0.25;
dot += offset;
This should give an additional effect where changes in direction close to being antiparallel to a given speaker should still have an effect on the volume of the audio from that speaker (whether or not that is actually audible is another question though) in contrast to the simple std::min approach, where the volume would plateau at 0.25 until that value is exceeded.

Does this all make some sense? 🤔

Boy the concept of sources, listener, and speaker direction is getting confusing. Right, so that would make volumes for directions close to antiparallel more distinct with the tradeoff that all directional volumes are slightly boosted. I guess that is fine either way (because you always have at least two speakers). I have implemented it how you suggested it.

Krzmbrzl

In calcGain we have

datt = powf(10.0f, log10f(mvol) * drel);

This can be simplified to datt = std::pow(10.0f, drel) * mvol; saving us a (comparatively) expensive call to log10.

Once you have tested the latest nit-picks of mine, feel free to merge without another review from me. I'm happy with how then code looks now :)

Krzmbrzl · 2023-10-23T07:25:10Z

src/mumble/AudioOutput.cpp

+ if (distance < 0.0001f) {
+ // Listener is inside source -> no attenuation
+ att = 1.0f;


Is this only to save unnecessary computation or has this extra branch a more subtle purpose?

If it is only about saving computation:
This equates to a distance of 0.0001m, which is 0.01cm. I think we can default to no attenuation way earlier. I would suggest 0.1m -> 0.1f.
Though in the end it would seem fine to me to just let the code fall-through to the blooming-branch below 👀

This branch covers #6234 (comment)

But we might get away with a larger value, yes.

Ah I see. However, to me this seems to indicate that the attenuation code doesn't "scale" properly - it should tend to 1 for distance -> 0. 🤔

As I said in the earlier comment, I assume this is because there is a inherent distance between the speakers of the player and the player position thus the distance will be close, but never exactly 0. And therefore, the volume will never be exactly 1.

But for distances small enough that this separate branch is taken, the fading function should be so close to 1 that we don't end up with an audible volume reduction. And given that we in fact do have such an audible reduction, I deduce that the fading function doesn't work the way we would expect it to...
Anyway, I'm happy with keeping the current approach. If someone fancies, the fading function problem can be dealt with in a separate PR.

I see what you mean. So the bloomfac in line 121 can have a maximum of Global::get().s.fAudioBloom, which is capped to 75% or something.
And then dotfactor may have a value of 0.5, because the dotproduct is probably 0. (since we have a directional zero vector). I do not see how 0.75 + 0.5 should be less than 1 though 🤔

Annnnyway, I think we should keep this branch in. Especially now that the check distance is larger.

It might be possible that I originally tried this with bloom 0. Which - still- should not decrease the volume when we are literally inside the source.

Right, that's actually it! If bloom < 0.5, we will get a volume < 1 for dist == 0, but as soon as we are not numerically inside the source, one of the channels will have a dotfactor of 1. That is the problem - a numerical error at exactly dist == 0 so the extra check for that case is probably warranted.

I think we could also get rid of the blooming now that we have the minimum volume on all speakers. I think blooming was effectively mimicking that. Anyway, this need not be done in this PR (if at all) :)

I think the bloom is meant to be a different effect. We should keep it as it produces significant changes to the perceived audio at close range to the source. The tiny minimum volume is more of a psycho-acoustic thing I suppose. I am not an audiophile though 🙃

src/mumble/AudioOutput.cpp

Hartmnt · 2023-10-24T17:35:11Z

This can be simplified to datt = std::pow(10.0f, drel) * mvol; saving us a (comparatively) expensive call to log10.

Are you sure about that? Quick testing does not seem to support that

… circumstances This commit does 4 things to improve a minimum positional volume of 0: 1) It checks, if the user selected a minimum volume of 0 and, if the audio source is exceeding the maximum distance. When both are true, it hard-codes the sample volume to 0 irrespective of other settings. 2) It changes the minimum gain calculation to be 25% of the maximum positional volume (irrespective of listener direction). This will make sure that low volume situations will not be boosted by the arbitrary 1/20 minimum that was used before, while still making sure that both ears receive some sound in most cases. 3) It reduces the min volume clamping from 0.01 to 0.005. This is done because with a minimum volume of 0 a hard cut could be heard when using the new logic from 1). By reducing the minimal clamp, we reduce the volume difference between the last possible step and 0. The value 0.005 was found experimentally with the manual placement plugin, a maximum distance of 20, and a minimum volume of 0. 4) It sets the volume gain factor to 1, if the listener is (almost) inside an audio source.

…ance constraints 8d7e1b5 added constraints to the positional distance sliders and spinner boxes. It was decided that the maximum distance must always be at least 1m larger than the minimum and vice versa. However, the methods which update the slider value were - incorrectly - using +1 and -1, which only equates to 0.1m in slide space. The subsequent update happening in the methods, which update the spinner boxes, were now using the incorrect 0.1m change in the checks and in turn adding or subtracting 1 in spinner box space (which correctly equates to 1m in spinner box space). Because of the order of loading operations, only the minimum slider is affected by the problem: It would reduce by 0.9m everytime the options dialog was loaded (and saved). This commit fixes the problem by adding and subtracting 10 in two of the methods which is 1m in slider space. It also slightly changes the wording of the comments

Krzmbrzl · 2023-10-24T19:18:47Z

Oops... no I was wrong. It would have been if there was a plus instead if a times in the exponent 🙈

Hartmnt · 2023-10-24T19:31:05Z

@Krzmbrzl Right, so if you are happy with the MR as is, feel free to merge. I did not test the change from dist < 0.0001f to dist < 0.01f, but I suppose it does not make a huge difference in the grand scheme of things.

Krzmbrzl · 2023-10-25T06:55:28Z

💔 All backports failed

Status	Branch	Result
❌	1.5.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

backport --pr 6234

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

Krzmbrzl · 2023-10-26T06:51:38Z

💚 All backports created successfully

Status	Branch	Result
✅	1.5.x

Questions ?

Please refer to the Backport tool documentation

…io fixes"

Hartmnt added client positional audio bug A bug (error) in the software backport-needed auto-backport-to-1.5.x labels Oct 10, 2023

Hartmnt requested a review from Krzmbrzl October 10, 2023 23:15

Krzmbrzl reviewed Oct 11, 2023

View reviewed changes

src/mumble/AudioOutput.cpp Outdated Show resolved Hide resolved

src/mumble/AudioOutput.cpp Outdated Show resolved Hide resolved

src/mumble/AudioOutput.cpp Outdated Show resolved Hide resolved

Hartmnt marked this pull request as draft October 11, 2023 11:58

Hartmnt force-pushed the fix_audio_min_volume branch from 8858aab to 50696b4 Compare October 11, 2023 11:59

Hartmnt force-pushed the fix_audio_min_volume branch 3 times, most recently from c3d5bd3 to 2f9f5c1 Compare October 11, 2023 12:41

Hartmnt marked this pull request as ready for review October 11, 2023 12:42

Hartmnt requested a review from Krzmbrzl October 11, 2023 12:49

Hartmnt force-pushed the fix_audio_min_volume branch 4 times, most recently from 84c5ed7 to 96db081 Compare October 11, 2023 22:25

Krzmbrzl approved these changes Oct 12, 2023

View reviewed changes

src/mumble/AudioOutput.cpp Outdated Show resolved Hide resolved

Hartmnt force-pushed the fix_audio_min_volume branch 3 times, most recently from 5d889b0 to 0634a37 Compare October 12, 2023 15:14

Hartmnt requested a review from Krzmbrzl October 12, 2023 15:33

Krzmbrzl approved these changes Oct 23, 2023

View reviewed changes

Hartmnt force-pushed the fix_audio_min_volume branch from 0634a37 to eea4dd8 Compare October 24, 2023 17:59

Hartmnt added 2 commits October 24, 2023 20:16

Hartmnt force-pushed the fix_audio_min_volume branch from eea4dd8 to 7f510bc Compare October 24, 2023 18:17

Krzmbrzl merged commit 8e9276b into mumble-voip:master Oct 25, 2023
15 checks passed

Hartmnt mentioned this pull request Oct 25, 2023

FEAT(client): Positional audio improvements #6134

Merged

1 task

Krzmbrzl mentioned this pull request Oct 26, 2023

Backport "Merge PR #6234: FIX(client): Positional audio fixes" to 1.5.x #6250

Merged

Krzmbrzl added a commit that referenced this pull request Nov 4, 2023

Merge PR #6250: Backport "Merge PR #6234: FIX(client): Positional aud…

d9bd3c4

…io fixes"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX(client): Positional audio fixes #6234

FIX(client): Positional audio fixes #6234

Hartmnt commented Oct 10, 2023

Hartmnt commented Oct 11, 2023 •

edited

Loading

Krzmbrzl commented Oct 12, 2023 •

edited

Loading

Hartmnt commented Oct 12, 2023

Krzmbrzl left a comment

Krzmbrzl Oct 23, 2023

Hartmnt Oct 23, 2023

Krzmbrzl Oct 23, 2023

Hartmnt Oct 23, 2023

Krzmbrzl Oct 24, 2023

Hartmnt Oct 24, 2023

Hartmnt Oct 24, 2023

Hartmnt Oct 24, 2023

Krzmbrzl Oct 25, 2023

Hartmnt Oct 25, 2023

Hartmnt commented Oct 24, 2023

Krzmbrzl commented Oct 24, 2023

Hartmnt commented Oct 24, 2023

Krzmbrzl commented Oct 25, 2023

Krzmbrzl commented Oct 26, 2023

FIX(client): Positional audio fixes #6234

FIX(client): Positional audio fixes #6234

Conversation

Hartmnt commented Oct 10, 2023

Hartmnt commented Oct 11, 2023 • edited Loading

Krzmbrzl commented Oct 12, 2023 • edited Loading

Hartmnt commented Oct 12, 2023

Krzmbrzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hartmnt commented Oct 24, 2023

Krzmbrzl commented Oct 24, 2023

Hartmnt commented Oct 24, 2023

Krzmbrzl commented Oct 25, 2023

💔 All backports failed

Manual backport

Questions ?

Krzmbrzl commented Oct 26, 2023

💚 All backports created successfully

Questions ?

Hartmnt commented Oct 11, 2023 •

edited

Loading

Krzmbrzl commented Oct 12, 2023 •

edited

Loading